Harden the Edge Cookie withdrawal write path - #1113
Open
prk-Jr wants to merge 12 commits into
Open
Conversation
Tombstone only an identity the graph already holds. The marker exists to stop later reads of a real row, so writing one for an identifier that was never issued enforces nothing while still consuming a write and a row, and the identifier arrives in a client-supplied cookie. Confirm existence with the list API rather than a lookup. A lookup is eventually consistent, so a stale miss would discard a genuine withdrawal; the list is strongly consistent. Reject anything that is not a well-formed EC ID before querying, since this is a prefix query and an empty or truncated value would match unrelated keys. When the list cannot answer, re-check with a lookup instead of writing regardless. Eventual consistency yields false negatives, never false positives, so a hit is proof the identity exists while no fabricated identifier can produce one. If neither can answer, report the withdrawal unconfirmed and write nothing; the browser cookie is expired either way and remains the primary enforcement. Split the unusable-consent branch out of ec_finalize_response and route the per-identity result through one place, so an unconfirmed identity is logged as a fault while an unknown one is not.
The degraded path dropped both the list and lookup errors, leaving a store outage undiagnosable. Log both, and use the raw lookup so a corrupt-but-present row is not read as absent. Add a test pinning the fixed-width assumption the prefix check relies on.
Counting keys by prefix reported a held identity whenever any longer key started with the one asked for, so a withdrawal for an identity that was never issued still wrote a row. Add an exact, strongly consistent `key_exists` to the store and use it. This also drops the dependency on the identifier grammar: the check no longer cares what shape an identifier takes, only whether that key is present. Redact the key in the Fastly lookup error, matching the list error. Carry the reason for an unconfirmed withdrawal so it is logged once.
Scanning one page of prefix matches assumed the exact key would be in it. Nothing guarantees that when other keys share the prefix, and stopping early reports a held identity as missing, discarding its withdrawal. Iterate the pages instead. Also correct two test comments that still described the removed grammar gate.
A byte index landing inside a multi-byte character makes `get` return `None`, and the fallback printed the whole identifier — the opposite of what the redaction is for. Truncate by character, and reuse the helper in the Fastly store rather than repeating the byte form there. Prove the bounds check avoids a store call with a counting backend, and rename the test that claimed to cover a grammar gate that no longer exists.
Contributor
|
Sequencing note on this PR and the provider stack. This PR and the open provider stack (#1043 to #1047, #1084, #1094) rework the same Edge Cookie finalize flow. A merge simulation of this PR's head (f6181d1, and identically its earlier head b5b68bb) against six of the seven stack heads conflicts in one file, Reproduce: The request is the one we have made on #885, #940 and #1094. The stack has been open since 19 August, carrying work that has been under review since 2 July as #838, is green on required CI, and its branches are kept rebased close to |
The exact-key scan lived in the Fastly store, where no test double can exercise paging. Extract it as `contains_exact_key` and have the backend supply pages to it, so multi-page matches, prefix-only keys, early exit and page errors are all covered natively.
Collaborator
Author
|
Closes #1116 |
prk-Jr
marked this pull request as draft
September 2, 2026 08:11
A third `Ok` variant was discarded by any caller inspecting only the error case, dropping a possibly-unrecorded withdrawal in silence. Returning `Err` keeps the underlying reports intact instead of flattening them into a string. Finish the redaction pass — insert, delete, and the deserialize paths still embedded the raw key — treat an empty prefix listing as absent rather than a failure, bound the pages an existence check will walk, and put the store trait's doc comment back on the trait.
The exact-key check followed a listing for a bounded number of pages and read running out of budget as absence. Absence is what tells the withdrawal path the identity was never issued, so a real identity on an unread page had its tombstone silently dropped — while the constant's own note and the tombstone docs both said the caller would treat that case as unconfirmed. Report it as a third outcome and map it to an error at the adapter, which puts it on the path that already re-checks by lookup. The page budget moves into the checked function so the listing is passed untruncated and there is no count to keep in agreement at the call site.
Nine messages interpolated the whole identifier: a duplicate create, upserts naming a missing or withdrawn key, and the CAS-exhaustion paths. Callers log these reports with debug formatting, so each one put a full identifier in a log line. The existing test only drove an injected backend failure, which never reaches them, so the module's claim that every message goes through the truncating helper held for the wrong reason. Route them through it too, and cover the paths a request can actually reach.
The test drove three of the message paths, so the other five held only by inspection. Extend it to the batched upserts and the three CAS-exhaustion terminal errors, which needed a conflict-injecting store that can hold a live entry rather than only a tombstone.
The conditional partner upsert's CAS-exhaustion error used the redacted template but no test executed it, so it was the one message still holding by inspection alone.
prk-Jr
marked this pull request as ready for review
September 3, 2026 06:01
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ts-eccookie named, whether or not the identity graph held that identity. The identifier is client-supplied and only shape-checked.How existence is determined
Worth reading before the diff, because the obvious approaches are wrong in ways that matter.
An exact check, not a prefix count. Counting keys by prefix reports a held identity whenever any longer key starts with the one asked for, so a withdrawal for an identity that was never issued would still write a row.
EcKvStoregainskey_exists, and the Fastly implementation lists by prefix and compares the returned keys for equality.Every page is followed. Nothing guarantees the exact key lands in the first page when other keys share its prefix, and stopping early would report a held identity as missing and discard its withdrawal.
The list, not a lookup. On Fastly a lookup is eventually consistent while
build_list()defaults toListMode::Strong. A stale lookup would report an identity issued moments earlier as missing.A raw lookup as fallback when the exact check fails. An unanswerable check is not evidence of absence, but writing regardless would restore the unconditional write whenever the store can be made to fail. A lagging lookup may miss a very recent write, and may return a row already deleted at the primary, but it cannot report an identifier this deployment never issued. The raw form is used because a row whose body no longer deserializes is still a row.
If neither can answer, nothing is written. The outcome is reported as unconfirmed with the reason, logged once at the request boundary. The browser cookie is expired in every case and remains the primary enforcement; only the batch-sync revocation marker is at stake.
Behaviour
write_withdrawal_tombstonereturns aTombstoneOutcome(Written/UnknownIdentity/Unconfirmed { reason }) rather than(), so the caller can tell expected traffic from a fault. An unknown identity is ordinary — the cookie is client-supplied — and logs at debug. An unconfirmed one means the store could not be read and logs at error.Changes
crates/trusted-server-core/src/ec/kv_backend.rsEcKvStore::key_exists— exact, strongly consistent existencecrates/trusted-server-core/src/ec/kv.rsTombstoneOutcome; bound the identifier length to cap the work a cookie can ask forcrates/trusted-server-core/src/ec/finalize.rsfinalize_unusable_consent(ec_finalize_response80 → 48 lines)crates/trusted-server-core/src/ec/mod.rslog_idtruncates by character — a byte index inside a multi-byte character made it emit the whole identifiercrates/trusted-server-adapter-fastly/src/ec_kv.rskey_existswith pagination; reuselog_idinstead of three local byte truncationsTest plan
cargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spintrusted-server-clicargo fmt --all -- --check19 tests added across both layers. The negative cases carry the regression protection; the happy-path ones exist to catch the gate over-blocking, which is the real risk here — a dropped opt-out is worse than an extra row. One test drives a store that cannot answer at all, asserting
Max-Age=0and header removal survive it.Known coverage gap: the pagination loop cannot be exercised from a test double, since the in-memory backend has no paging. It rests on the SDK contract and code review.
Closes
Closes #1116
Reviewer note — three PRs touch this code
is_valid_ec_idto the built-in provider's grammar. The KV gate no longer depends on it, so that is retired insidekv.rs— but resolving the finalize-side merge toward this branch'swithdrawal_ec_idswould reinstate the grammar filter a layer up and drop vendor identities.if let Errat the shared call site still compiles against the three-variant return, so a merge toward that version drops the outcome handling with no compile error.Checklist
unwrap()in production codelogmacros (notprintln!)